cl/forkchoice: harden Gloas envelope persistence - #23152

Gossip dedup is now bypassable. Adding && s.forkchoiceStore.HasEnvelope(beaconBlockRoot) means the seen-cache no longer short-circuits on its own.

HasEnvelope is false whenever the root sits in invalidEnvelopes (any structurally-corrupt read marks it, and the file is deliberately kept) or whenever envelopeExists was evicted. In that state every replay of the same envelope from the same builder falls through to OnExecutionPayload, which re-runs BLS verification and engine_newPayload, and re-emits execution_payload_available on SSE. A peer that can get one root into that state gets unbounded CPU + EL amplification from a single stored envelope.

If the goal is "retry when persistence failed", key that on a persistence-failure marker rather than on the general availability predicate.

The envelope file is removed after stateDumpLock is released, so the cache/file invariant the PR sets out to establish still has a window.

envelopeExists.Delete(root) happens on line 621 under the lock, but Remove(getEnvelopeFilename(root)) runs after Unlock(). Between the two, a DumpEnvelopeOnDisk for that root can acquire the lock, pass knowsBlockRoot (the root is re-added to f.blocks by AddChainSegment, which stores the block early at fork_graph_disk.go:305 with no lowest-slot gate), rename its temp into place and set envelopeExists. Prune then deletes the file it just wrote.

Result: HasEnvelope(root) == true with no file on disk, and every subsequent ReadEnvelopeFromDisk fails with ErrNotExist — the sentinel ExecutionPayloadEnvelopesByRoot handler advertises the envelope and then errors.

Moving both Remove calls inside the same critical section as the map deletes closes it.

These four lists are progressive in Gloas, so a hard len <= configured max check rejects what the decoder accepts.

For Gloas, ensureLists builds all five via NewStaticProgressiveListSSZ, whose limit is progressiveDecodeLimit(configLimit) = 2 * configLimit. progressiveDecodeLimit's own comment says: "Progressive lists are semantically unbounded, so decode limits are resource guards rather than protocol maxima." validateForPersistence follows that for Deposits (ValidateProgressiveDecodeBounds, i.e. 2x), but lines 240-251 apply the raw 1x protocol maximum to the other four.

Two consequences:

• Inconsistent: an envelope with MaxDepositRequestsPerPayload + 1 deposits is accepted, one with MaxWithdrawalRequestsPerPayload + 1 withdrawals is rejected.
• Read-side amplification: ReadEnvelopeFromDisk also runs this, and a failure sets invalidEnvelopes permanently. Any config tightening of these four maxima turns already-persisted envelopes into permanently unreadable ones and kills HasEnvelope for those roots.

Per cl/CLAUDE.md ("Review all changes against the upstream Ethereum consensus specifications"), either use ValidateProgressiveDecodeBounds for all five or drop the four 1x checks.

Unsynchronised map write, plus a nil-map panic on literal-constructed mocks.

ForkChoiceStorageMock has no mutex. executionPayloadService runs go s.loop(ctx) from its constructor; processPendingEnvelopes calls ProcessMessage -> OnExecutionPayload (this write) while the test goroutine calls ProcessMessage -> HasEnvelope, which reads f.Envelopes at line 442. That is a concurrent map read + write, which is a Go runtime fatal error, not just a -race report.

Separately, Envelopes is only allocated in NewForkChoiceStorageMock. Several tests build the mock as a literal (&mock_services.ForkChoiceStorageMock{} in cl/sentinel/handlers/blocks_by_root_test.go:91, blobs_test.go:116, rate_limiter_integration_test.go:65, ...). Any of those reaching OnExecutionPayload panics with "assignment to entry in nil map".

Guard the write with a mutex and lazily allocate the map.

clparams.MaxChunkSize is the package constant (mainnet 15 MiB), not the per-network f.beaconCfg.MaxChunkSize.

MAX_CHUNK_SIZE is a configurable preset — clparams.BeaconChainConfig carries it as a yaml-bound field (config.go:225) and networks can and do override it. Using the compile-time constant here (and at lines 346 and 368, and in readEnvelopeHTTPBody in both beacon_downloader.go and remote_checkpoint_sync.go) silently applies mainnet's bound to every chain.

f.beaconCfg / b.beaconCfg / r.beaconConfig are in scope at every one of those sites.

The detach drops the decoder limits that Eth1Block.decodeSSZ installed.

Decoding builds the field as solid.NewTransactionsSSZWithLimits(cfg.MaxTransactionsPerPayload, cfg.MaxBytesPerTransaction); NewTransactionsSSZFromTransactions sets only underlying, leaving both limits zero. maxTransactions() / maxBytes() then fall back to MaxTransactionsPerPayloadDefault (1<<20) and MaxBytesPerTransactionDefault (1<<30).

The returned envelope is handed to the sentinel handlers and to chain_tip_sync, and Eth1Block.Clone() round-trips it through DecodeSSZ — that re-decode now uses the defaults instead of this chain's configured maxima.

A NewTransactionsSSZFromTransactionsWithLimits (or copying the two fields onto the new value) keeps the detach without losing the bounds.

Prune now blocks on stateDumpLock and performs file I/O under it, on the fork-choice drain path.

Prune is called from drainQueuedWork (cl/phase1/forkchoice/utils.go:69), which runs right after the fork-choice mutex is released. stateDumpLock is held for the whole of DumpBeaconStateOnDisk / readBeaconStateFromDisk — a mainnet beacon state is hundreds of MB of encode + snappy + Sync(). Previously Prune took no lock at all; now each of up to SlotsPerEpoch-many roots waits behind one of those, and f.fs.Remove(getBeaconStateFilename(root)) runs inside the critical section too.

Only the map mutations need the lock — hoist f.fs.Remove out (together with the envelope removes, see the other comment on this loop), or take the lock once for the whole map-mutation pass.

A fresh snappy.Reader per envelope read.

snappy.NewReader allocates ~141 KB (a 76 KB buf plus a 65 KB decoded). This path is already serialised by stateDumpLock, exactly like readBeaconStateFromDisk which reuses the pooled f.sszSnappyReader via Reset (line 70). The sentinel ExecutionPayloadEnvelopesByRange/ByRoot handlers call this once per served envelope.

Add an envelopeSnappyReader *snappy.Reader field and Reset(readTracker) it, mirroring the state path.

Duplicated helper. readEnvelopeHTTPBody is byte-identical to the one added at cl/phase1/core/checkpoint_sync/remote_checkpoint_sync.go:199.

Both read an envelope response under the same bound. Put one copy somewhere both packages can reach (e.g. next to the envelope types in cl/cltypes, or a small helper in cl/clparams/cl/utils) so the bound cannot drift between the two fetch paths.

Both !wasCached eviction branches are no-ops.

wasCached is exactly envelopeExists.Load(blockRoot) taken at line 228, under the same lock, and nothing stores the key before these branches run. So when !wasCached the key is by definition absent and Delete does nothing.

Line 239 reduces to if errors.Is(err, os.ErrNotExist), and the else if err != nil && !wasCached arm in the deferred func (line 251) is dead in full. Dropping them makes the actual policy — "evict only on ErrNotExist or structural corruption" — readable.

This can never fire for SSZ-decoded extra data.

SetBytes clamps: if e.l > maxExtraDataBytes { e.l = len(e.data) }, and NewExtraData allocates data at exactly maxExtraDataBytes. So after any DecodeSSZ, e.l <= 32 always, and oversized SSZ extra data is silently truncated rather than reported here.

Only UnmarshalJSON (which replaces e.data wholesale) can produce e.l > 32, so the check only covers the REST/JSON path. If the intent is to catch oversized SSZ extra data, the guard belongs in SetBytes/DecodeSSZ returning an error, not in a post-hoc bounds check that the setter has already made unreachable.

The bound is taken as a parameter while the instance already carries b.limit.

ByteListSSZ is constructed with its own limit (and DecodeSSZ/SetBytes enforce it). Taking a second, unrelated limit here means a caller that passes a different value than the one used at construction validates against the wrong maximum with no signal — and ValidateForPersistence does exactly that, passing cfg.MaxBytesPerTransaction to a list that may have been built from another config.

Either validate against b.limit and drop the parameter, or assert the two agree.

Asymmetric error handling in the same loop. The two envelope removes below now join their errors into the returned err, but the beacon-state remove on this line still discards its result.

A failing state-file remove leaks a multi-hundred-MB file per pruned root with no signal anywhere. If removal failures are worth surfacing for envelopes, they are worth surfacing for the much larger state files.

Redundant pre-check. Line 368 already rejects on len(f.sszBuffer) > MaxChunkSize using the actual encoded length, which is the authoritative value.

This earlier EncodingSizeSSZ() check adds a second, weaker bound and is not free of side effects: Eth1Block.EncodingSizeSSZ lazily assigns b.Withdrawals / b.BlockAccessList and ExecutionRequests.EncodingSizeSSZ calls ensureLists(), so the caller's envelope is mutated during what reads as a validation step. Drop it and keep the post-encode check.

Wall-clock synchronisation makes these prune tests flaky on loaded CI.

This time.Sleep(10ms) is meant to let the DumpEnvelopeOnDisk goroutine reach stateDumpLock; on a contended runner it may not have been scheduled yet, and the test then asserts the wrong thing. TestPruneDoesNotRaceEnvelopeReplacement has the mirror problem — its time.After(time.Second) arm makes the test always burn a full second on the expected path.

Both are expressible with the channel handshake the fixtures already provide (envelopeBlockingPruneFs.firstReached / releaseFirst): add a channel the dump goroutine closes once it is inside DumpEnvelopeOnDisk, and wait on that instead.